Submit delete batches in parallel#3123
Conversation
poodlewars
force-pushed
the
deletion-cleanup
branch
from
5094aad to
25c3c5a
Compare
ArcticDB Code Review SummaryLatest commit Build and Dependencies
API and Compatibility
Documentation
|
poodlewars
force-pushed
the
deletion-cleanup
branch
2 times, most recently
from
9852aeb to
0614e82
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
poodlewars
force-pushed
the
deletion-cleanup
branch
from
c7ded90 to
31f99ea
Compare
| ); | ||
| return folly::collect(std::move(futs)).via(&async::io_executor()).thenValue([](auto&&) { | ||
| return std::vector<RemoveKeyResultType>{}; | ||
| }); |
There was a problem hiding this comment.
https://github.com/man-group/ArcticDB/blob/master/cpp/arcticdb/stream/stream_sink.hpp#L49-L54
Don't have to, but if you want to rip out this pointless abstraction while you're here I won't object
| keys = {}; | ||
| keys.reserve(flush_threshold); | ||
| const auto batch_size = batch.size(); | ||
| store->remove_keys(std::move(batch)).get(); |
There was a problem hiding this comment.
There is a slightly more elegant model that would remove the need for flush_threshold . Instead of calling get immediately, we maintain a set of remove_keys futures we've submitted. Folly let's you check if a future is done without calling get() using isReady(), so the size of this collection could be kept bounded. It's quite a bit more complicated though, and probably doesn't add much in this case.
At least worth a comment about why remove_keys is better than remove_keys_sync in this case though.
| with ( | ||
| config_context("S3Storage.DeleteBatchSize", batch_size), | ||
| config_context("AzureStorage.DeleteBatchSize", batch_size), | ||
| ): |
There was a problem hiding this comment.
There's a config_context_multi for setting multiple options at once
|
|
||
|
|
||
| @pytest.mark.storage | ||
| def test_bulk_delete_batching(object_and_mem_and_lmdb_version_store, sym): |
There was a problem hiding this comment.
This is quite an indirect test of the batching. I get that we don't have query stats for Azure right now, but the test below is much more explicit
poodlewars
force-pushed
the
deletion-cleanup
branch
from
31f99ea to
7651360
Compare
3 participants
Discovered while working on delayed deletes. I created a library on PURE with 1000 symbols, with ~200 versions each and 10 snapshots.
Then running delayed deletes had these results:
Notably running delayed deletes in "dry run" mode (which skips the physical deletion) showed no performance difference between arcticc and ArcticDB, for example on a small example:
arcticc submitted
RemoveBatchTaskin chunks of 1000 https://mangit.maninvestments.com/projects/DATA/repos/arcticc/browse/arcticcxx_impl/async/async_store.hpp#238 but #70 changed it to this implementation.If we delete 10k keys, the old implementation in arcticc would submit 10 tasks to delete 1000 keys in parallel, whereas ArcticDB currently submits 10 HTTP requests to delete 1000 keys in serial.
This explains most of the performance difference between arcticc delayed deletes and enterprise delayed deletes.
To stop storage specific deletion batch size limits leaking out of the storage layer, this PR has each storage declare its maximum deletion batch size, which is then used by the
AsyncStorewhen it calculates the batches.